-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes Nested Tags Search Error #4651
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4651 +/- ##
=======================================
Coverage 97.06% 97.07%
=======================================
Files 301 301
Lines 17878 17884 +6
=======================================
+ Hits 17354 17361 +7
+ Misses 420 419 -1
Partials 104 104
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>
06e960c
to
db2245e
Compare
@yurishkuro , as discussed in #3900 , I have raised this PR. Could you please take a look and give pointers on improvement? |
} | ||
// Nested Logs Query |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
explain why this is separate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we had all the nested fields query together initally (tags , process tags , logs) . When we set AllTagsAsFields as true , we still want to query the nested logs field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it because with AllTagsAsFields=true the tags from logs are still written as nested? Then how does this fix prevent the reported issue? When a span has no logs, it has no tags:[]
in the document, but the query would still be looking for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, when AllTagsAsFields is true, logs are written as nested field. Arent logs and tags two independent fields in span context ? The change that I made does not search in the tags field and the process.tags field when allTagsAsFields is true. The reported issue said that they had a different mapping configuration than the jaeger default configuration and hence it never populated the tags field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's my point - we should have a test that reproduces the reported issue in the first place (maybe as a new integration test), before we try fixing it. Otherwise how do you know that these changes are fixing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understand that. Its however redundant to query the tags field when all tags as fields is true. I will work on the integration test!
@@ -168,6 +168,7 @@ func (s *ESStorageIntegration) initSpanstore(allTagsAsFields, archive bool) erro | |||
IndexPrefix: indexPrefix, | |||
MaxSpanAge: maxSpanAge, | |||
TagDotReplacement: tagKeyDeDotChar, | |||
AllTagsAsFields: allTagsAsFields, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can there be a test that validates this new behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integration tests were strong and gave the expected behaviour. I can add the unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but those tests didn't catch the issue reported by user?
@pmuls99 please address Yuri's comments or this will be closed out. |
Which problem is this PR solving?
Description of the changes